Fix multi-buffer data loss in DataBufferUtils.write#36714
Open
cookie-meringue wants to merge 1 commit intospring-projects:mainfrom
Open
Fix multi-buffer data loss in DataBufferUtils.write#36714cookie-meringue wants to merge 1 commit intospring-projects:mainfrom
cookie-meringue wants to merge 1 commit intospring-projects:mainfrom
Conversation
Prior to this commit, WritableByteChannelSubscriber.hookOnNext() called iterator.next() exactly once. If a DataBuffer consisted of multiple NIO ByteBuffers (e.g., NettyDataBuffer wrapping a CompositeByteBuf), only the first buffer was written to the channel, and the remaining buffers were silently ignored and lost. This commit adds the missing while (iterator.hasNext()) outer loop to ensure all fragmented buffers exposed by the iterator are completely and safely written to the synchronous channel. Signed-off-by: KimDaehyeon <daehyeon3351@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a bug in
DataBufferUtils.write(Publisher<DataBuffer>, WritableByteChannel)where only the first ByteBuffer is written and the remaining data is silently discarded when writing a DataBuffer composed of multiple NIO ByteBuffers to a synchronous channel.Conditions & Root Cause
The core of this bug lies in a violation of the
DataBuffer.ByteBufferIteratorcontract, rather than a flaw in a specific data structure.When writing data to a channel, the
WritableByteChannelSubscriber.hookOnNext()method inDataBufferUtils.write()operates as follows:According to the
DataBuffer's Javadoc, the iterator returned bydataBuffer.readableByteBuffers()exposes each ByteBuffer, and the caller is responsible for iterating through them all usingwhile (iterator.hasNext()).However, the current
WritableByteChannelSubscriber.hookOnNext()callsiterator.next()exactly once without an outer loop.Consequently, if the iterator exposes two or more elements (N > 1), the second and all subsequent ByteBuffers are never accessed.
They are released and become inaccessible as the try-with-resources block closes, resulting in silent data loss.
Fix
Add the missing outer
while (iterator.hasNext())loop inWritableByteChannelSubscriber#hookOnNextso that allByteBuffers exposed by the iterator are written:Tests
Added
writeWritableByteChannelWithJoinedBuffertoDataBufferUtilsTests. The test joins two DataBuffers viabufferFactory.join(List.of(foo, bar)), writes the resulting DataBuffer to a synchronous channel, and asserts the file contains"foobar".Background
This bug was introduced during the refactoring in commit 3e2f58c
During the migration from
dataBuffer.toByteBuffer()(which guaranteed a single buffer) todataBuffer.readableByteBuffers()(which exposes multiple buffers) for zero-copy optimization, the existing single-buffer processing pattern of the synchronous write path was left unchanged, omitting the necessary outer iteration loop.Notably, the asynchronous sibling
WriteCompletionHandler(in the same file, migrated in the same commit) iterates correctly via its callback chain — further suggesting that the missing loop in the synchronous path is an oversight rather than intent.Existing callers follow the iterator contract
All other callers of
readableByteBuffers()in the codebase iterate correctly using thewhile (iterator.hasNext())pattern:JacksonTokenizerJackson2TokenizerXmlEventDecoderPartGeneratorJettyClientHttpRequestServletServerHttpResponseStandardWebSocketSessionImpact
Multi-element iterators are produced wherever
NettyDataBufferFactory.join(List<DataBuffer>)is invoked, which is the standard way Spring combines multiple DataBuffers into one. This is used pervasively across WebFlux:DataBufferUtils.jointo buffer the entire body before parsing): Jackson JSON/XML/CBOR/Smile, Gson, Kotlin Serialization, Protobuf, Form, Multipart.bufferFactory.jointo prepend prefixes/separators): Jackson encoders, Gson, Protobuf JSON.ViewResolutionResultHandler(fragment joining),CssLinkResourceTransformer,ContentVersionStrategy.The bug surfaces when the resulting
DataBufferis subsequently written to a synchronousWritableByteChannel. The asynchronousAsynchronousFileChannelpath (WriteCompletionHandler) is unaffected.